lib: add support for JSTransferable as a mixin #38383

jasnell · 2021-04-24T14:58:24Z

@addaleax ... Very interested in what you think on this... the use case is that I want to define internal classes that are both transferable and extend NodeEventTarget but without forcing all NodeEventTarget instances to be cloneable or incur the cost of extending JSTransferable. Because we can't use multiple inheritance this uses JSTransferable as a kind of mixin.

This is not a public facing API. It is inteded for internal use only for now.

Adds a new makeTransferable() utility that can construct a
JSTransferable object that does not directly extend the
JSTransferable JavaScript class.

Because JavaScript does not support multiple inheritance, it is
not possible (without help) to implement a class that extends
both JSTransferable and, for instance, EventTarget without
incurring a significant additional complexity and performance
cost by making all EventTarget instances extend JSTransferable...

That is, we don't want:

class EventTarget extends JSTransferable { ... }

The makeTransferable() allows us to create objects that are
backed internally by JSTransferable without having to actually
extend it by leveraging the magic of Reflect.construct().

const {
  JSTransferable,
  kClone,
  kDeserialize,
  makeTransferable,
} = require('internal/worker/js_transferable');

class E {
  constructor(b) {
    this.b = b;
  }
}

class F extends E {
  constructor(b) {
    super(b);
    return makeTransferable(this);
  }

  [kClone]() { /** ... **/ }
  [kDeserialize]() { /** ... **/ }
}

const f = new F();

f instanceof F;  // true
f instanceof E;  // true
f instanceof JSTransferable;  // false

const mc = new MessageChannel();
mc.port1.onmessage = ({ data }) => {
  data instanceof F;  // true
  data instanceof E;  // true
  data instanceof JSTransferable;  // false
};
mc.port2.postMessage(f);  // works!

The additional internal/test/transfer.js file is required for the
test because successfully deserializing transferable classes requires
that they be located in lib/internal for now.

Signed-off-by: James M Snell jasnell@gmail.com

addaleax · 2021-04-24T19:56:23Z

@jasnell I think this is fine to do, yes 👍

devsnek · 2021-04-24T20:21:53Z

maybe i'm misunderstanding what kConstructor does but would it be possible to instead of doing the following:

class X {
  constructor() {
    this.a = 1;
  }
  [kConstructor]() { return makeTransferable(X) }
}

do this?

class X {
  constructor() {
    this.a = 1;
    return makeTransferable(this);
  }
}

jasnell · 2021-04-25T00:25:57Z

maybe i'm misunderstanding what kConstructor does but would it be possible to instead of doing the following:

Yep, absolutely could. I wasn't yet sure if we'd always want X to always be transferable but I've been going through a range of cases and using the simpler option makes sense :-)

jasnell · 2021-04-25T00:43:02Z

@devsnek ... updated to use the simpler constructor form! This should be ready for review.

lib/internal/worker/js_transferable.js

Adds a new `makeTransferable()` utility that can construct a `JSTransferable` object that does not directly extend the `JSTransferable` JavaScript class. Because JavaScript does not support multiple inheritance, it is not possible (without help) to implement a class that extends both `JSTransferable` and, for instance, `EventTarget` without incurring a significant additional complexity and performance cost by making all `EventTarget` instances extend `JSTransferable`... That is, we *don't* want: ```js class EventTarget extends JSTransferable { ... } ``` The `makeTransferable()` allows us to create objects that are backed internally by `JSTransferable` without having to actually extend it by leveraging the magic of `Reflect.construct()`. ```js const { JSTransferable, kClone, kDeserialize, kConstructor, makeTransferable, } = require('internal/worker/js_transferable'); class E { constructor(b) { this.b = b; } } class F extends E { [kClone]() { /** ... **/ } [kDeserialize]() { /** ... **/ } static [kConstructor]() { return makeTransferable(F); } } const f = makeTransferable(F, 1); f instanceof F; // true f instanceof E; // true f instanceof JSTransferable; // false const mc = new MessageChannel(); mc.port1.onmessage = ({ data }) => { data instanceof F; // true data instanceof E; // true data instanceof JSTransferable; // false }; mc.port2.postMessage(f); // works! ``` The additional `internal/test/transfer.js` file is required for the test because successfully deserializing transferable classes requires that they be located in `lib/internal` for now. Signed-off-by: James M Snell <jasnell@gmail.com>

nodejs-github-bot · 2021-04-25T22:45:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37718/ 💚

Adds a new `makeTransferable()` utility that can construct a `JSTransferable` object that does not directly extend the `JSTransferable` JavaScript class. Because JavaScript does not support multiple inheritance, it is not possible (without help) to implement a class that extends both `JSTransferable` and, for instance, `EventTarget` without incurring a significant additional complexity and performance cost by making all `EventTarget` instances extend `JSTransferable`... That is, we *don't* want: ```js class EventTarget extends JSTransferable { ... } ``` The `makeTransferable()` allows us to create objects that are backed internally by `JSTransferable` without having to actually extend it by leveraging the magic of `Reflect.construct()`. ```js const { JSTransferable, kClone, kDeserialize, kConstructor, makeTransferable, } = require('internal/worker/js_transferable'); class E { constructor(b) { this.b = b; } } class F extends E { [kClone]() { /** ... **/ } [kDeserialize]() { /** ... **/ } static [kConstructor]() { return makeTransferable(F); } } const f = makeTransferable(F, 1); f instanceof F; // true f instanceof E; // true f instanceof JSTransferable; // false const mc = new MessageChannel(); mc.port1.onmessage = ({ data }) => { data instanceof F; // true data instanceof E; // true data instanceof JSTransferable; // false }; mc.port2.postMessage(f); // works! ``` The additional `internal/test/transfer.js` file is required for the test because successfully deserializing transferable classes requires that they be located in `lib/internal` for now. Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #38383 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Khaidi Chu <i@2333.moe>

jasnell · 2021-04-26T14:59:31Z

Landed in bbe24e2

Adds a new `makeTransferable()` utility that can construct a `JSTransferable` object that does not directly extend the `JSTransferable` JavaScript class. Because JavaScript does not support multiple inheritance, it is not possible (without help) to implement a class that extends both `JSTransferable` and, for instance, `EventTarget` without incurring a significant additional complexity and performance cost by making all `EventTarget` instances extend `JSTransferable`... That is, we *don't* want: ```js class EventTarget extends JSTransferable { ... } ``` The `makeTransferable()` allows us to create objects that are backed internally by `JSTransferable` without having to actually extend it by leveraging the magic of `Reflect.construct()`. ```js const { JSTransferable, kClone, kDeserialize, kConstructor, makeTransferable, } = require('internal/worker/js_transferable'); class E { constructor(b) { this.b = b; } } class F extends E { [kClone]() { /** ... **/ } [kDeserialize]() { /** ... **/ } static [kConstructor]() { return makeTransferable(F); } } const f = makeTransferable(F, 1); f instanceof F; // true f instanceof E; // true f instanceof JSTransferable; // false const mc = new MessageChannel(); mc.port1.onmessage = ({ data }) => { data instanceof F; // true data instanceof E; // true data instanceof JSTransferable; // false }; mc.port2.postMessage(f); // works! ``` The additional `internal/test/transfer.js` file is required for the test because successfully deserializing transferable classes requires that they be located in `lib/internal` for now. Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #38383 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Khaidi Chu <i@2333.moe>

targos · 2021-05-03T16:24:09Z

I'm removing the semver-minor label as:

This is not a public facing API. It is inteded for internal use only for now.

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. worker Issues and PRs related to Worker support. labels Apr 24, 2021

jasnell marked this pull request as ready for review April 25, 2021 00:40

This comment has been minimized.

Sign in to view

addaleax reviewed Apr 25, 2021

View reviewed changes

lib/internal/worker/js_transferable.js Outdated Show resolved Hide resolved

jasnell force-pushed the jstransferable_mixin branch 2 times, most recently from 5ca4d52 to c915542 Compare April 25, 2021 15:42

This comment has been minimized.

Sign in to view

jasnell force-pushed the jstransferable_mixin branch from c915542 to ad99a04 Compare April 25, 2021 15:58

This comment has been minimized.

Sign in to view

addaleax approved these changes Apr 25, 2021

View reviewed changes

XadillaX approved these changes Apr 26, 2021

View reviewed changes

jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Apr 26, 2021

jasnell closed this Apr 26, 2021

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 26, 2021

targos removed the semver-minor PRs that contain new features and should be released in the next minor version. label May 3, 2021

targos mentioned this pull request May 3, 2021

v16.1.0 release proposal #38522

Merged

targos added the backport-blocked-v14.x label May 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: add support for JSTransferable as a mixin #38383

lib: add support for JSTransferable as a mixin #38383

jasnell commented Apr 24, 2021 •

edited

Loading

addaleax commented Apr 24, 2021

devsnek commented Apr 24, 2021 •

edited

Loading

jasnell commented Apr 25, 2021

jasnell commented Apr 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Apr 25, 2021 •

edited by jasnell

Loading

jasnell commented Apr 26, 2021

targos commented May 3, 2021

lib: add support for JSTransferable as a mixin #38383

lib: add support for JSTransferable as a mixin #38383

Conversation

jasnell commented Apr 24, 2021 • edited Loading

addaleax commented Apr 24, 2021

devsnek commented Apr 24, 2021 • edited Loading

jasnell commented Apr 25, 2021

jasnell commented Apr 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Apr 25, 2021 • edited by jasnell Loading

jasnell commented Apr 26, 2021

targos commented May 3, 2021

jasnell commented Apr 24, 2021 •

edited

Loading

devsnek commented Apr 24, 2021 •

edited

Loading

nodejs-github-bot commented Apr 25, 2021 •

edited by jasnell

Loading